Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add -config flag #18

Closed
wants to merge 1 commit into from
Closed

Add -config flag #18

wants to merge 1 commit into from

Conversation

yonas
Copy link
Contributor

@yonas yonas commented Aug 20, 2024

Allow users to specify path to config.json.

@yonas yonas mentioned this pull request Aug 20, 2024
Copy link
Owner

@TimoKats TimoKats left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will make some commits to later to fix some things probably. Thanks again btw for sharing this idea, it's really good. I definitely want to implement it.

Comment on lines +68 to +71
case 2:
cmd = os.Args[1]
case 4:
cmd = os.Args[3]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something I'm not a huge fan of; because it's guessing whether the config flag comes before or after the command.

Comment on lines +25 to +27
func ReadConfig(config_path string) (Config, error) {
var config Config
configPath := getConfigPath()
configPath := getConfigPath(config_path)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Golang I use camelCase instead of underscores for variables (as is mentioned in Google's style guide).

https://google.github.io/styleguide/go/guide#mixed-caps

Comment on lines +9 to +15
func getConfigPath(config_path string) string {
if FileExists(config_path) {
return config_path
} else {
dirname, _ := os.UserHomeDir()
return dirname + "/.mdrss/config.json"
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I check for the existence of the config path later in the code. Here I just select / construct the config path that I want to use. Hence. the first if statement should only check whether the config path from the flag is enabled.

(PS: it should not be called configPath yet, because we don't know whether it is even used at this point. It's really the config path flag).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also add that XDG should be used. So this should be ~/.config/mdrss/config.json.

Instead of:

dirname, _ := os.UserHomeDir()
return dirname + "/.mdrss/config.json"

you can use adrg/xdg:

configFilePath, err := xdg.ConfigFile("mdrss/config.json")
if err != nil {
 log.Fatal(err)
}
return configFilePath

@yonas
Copy link
Contributor Author

yonas commented Aug 20, 2024

Hi @TimoKats, I'll leave the PR in your hands to change the way you like before merge.

@TimoKats
Copy link
Owner

Hi @yonas . Thanks will do

@TimoKats TimoKats closed this Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants